Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Soc descriptor harvesting #202

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Soc descriptor harvesting #202

merged 1 commit into from
Oct 29, 2024

Conversation

pjanevskiTT
Copy link
Contributor

@pjanevskiTT pjanevskiTT commented Oct 22, 2024 •
edited
Loading

Fix #102, part of a general effort around soc descriptor #100

Goal of the PR is to move harvesting logic to soc descriptor, with clear interface for coordinate translation

Finished TODOs

  • Derive logical, virtual and physical coordinates from tt_xy_pair
  • Make soc descriptor tests arch independent as much as possible
  • Cleanup soc desc code for harvesting on y or x based on the arch
  • Revert commented tests
  • Move open UMD doc about harvesting to UMD repo
  • Add tests for grayskull
  • Full interface to convert between cordinate system

First class TODOs (will finish in this PR)

  • Blackhole to translated coordinates fix
  • cpp nits

UMD client changes

No changes needed for bumping

TODOs with follow up issues (won't fix in this PR)

@pjanevskiTT pjanevskiTT self-assigned this Oct 22, 2024
abhullar-tt
abhullar-tt reviewed Oct 22, 2024
View reviewed changes
device/tt_soc_descriptor.cpp Outdated Show resolved Hide resolved
tests/blackhole/test_soc_descriptor.cpp Outdated Show resolved Hide resolved
device/tt_soc_descriptor.h Outdated Show resolved Hide resolved
@abhullar-tt
Copy link
Contributor

is the idea for tt_xy_pair to have an additional field to denote coordinate type (logical, virtual, physical, etc?)

@pjanevskiTT
Copy link
Contributor Author

is the idea for tt_xy_pair to have an additional field to denote coordinate type (logical, virtual, physical, etc?)

I am probably going to derive each type of coordinates from tt_xy_pair, so instead of

using tt_logical_coords = tt::umd::tt_xy_pair;

we are going to have derive tt_logical_coords from tt_xy_pair, so you still have only x and y as fields, but you can use coordinates as types, you will know which coordinates you are using just based on the type of the variable. Also, we want to be sure to not use the API in a way it is not intended, we can provide API for each type of coordinates. Does that make more sense?

joelsmithTT
joelsmithTT reviewed Oct 22, 2024
View reviewed changes
Copy link
Contributor

@joelsmithTT joelsmithTT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have an accurate mental model for the coordinate systems.. I suspect I am not the only person with this problem. I think that clear definitions of what each of the coordinate systems represent (perhaps with some examples) could go a long way for helping developers build an accurate understanding of coordinates and harvesting.

device/tt_xy_pair.h Outdated Show resolved Hide resolved
@abhullar-tt
Copy link
Contributor

is the idea for tt_xy_pair to have an additional field to denote coordinate type (logical, virtual, physical, etc?)

I am probably going to derive each type of coordinates from tt_xy_pair, so instead of

using tt_logical_coords = tt::umd::tt_xy_pair;

we are going to have derive tt_logical_coords from tt_xy_pair, so you still have only x and y as fields, but you can use coordinates as types, you will know which coordinates you are using just based on the type of the variable. Also, we want to be sure to not use the API in a way it is not intended, we can provide API for each type of coordinates. Does that make more sense?

yes that makes sense, thanks!

@pjanevskiTT pjanevskiTT force-pushed the pjanevski/soc_desc_harvesting branch 5 times, most recently from 94355d4 to 7f5e04a Compare October 23, 2024 09:12
@pjanevskiTT pjanevskiTT marked this pull request as ready for review October 23, 2024 15:17
@pjanevskiTT pjanevskiTT mentioned this pull request Oct 23, 2024
Open
@pjanevskiTT pjanevskiTT added the changes api API changing PR, needs changes in client code label Oct 24, 2024
@pjanevskiTT pjanevskiTT force-pushed the pjanevski/soc_desc_harvesting branch 2 times, most recently from a508dac to 950feee Compare October 24, 2024 13:09
@pjanevskiTT pjanevskiTT mentioned this pull request Oct 25, 2024
Open
2 tasks
@pjanevskiTT
Copy link
Contributor Author

pjanevskiTT commented Oct 25, 2024 •
edited
Loading

I know this PR is becoming a bit hard to review with a lot of code, so I will separate TODOs left into first class that I will solve in this PR and then merge it, and into second class tasks that I will open issues for and solve it all as separate PRs. I tried thinking about making this PR smaller as well, but I think this is a good base (coordinate conversion API, tests, docs) for harvesting so we can build on top of it. Uplifting this is also going to be seamless since it doesn't change or removes the API, it just adds the functions, so I can make the API function for tt-metal in the following commits.

@broskoTT @joelsmithTT let me know if this is ok for you

joelsmithTT
joelsmithTT reviewed Oct 25, 2024
View reviewed changes
docs/coordinate_systems.md Outdated Show resolved Hide resolved
tests/soc_descs/grayskull_10x12.yaml Outdated Show resolved Hide resolved
joelsmithTT
joelsmithTT approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@joelsmithTT joelsmithTT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to get this in and refine later, that is good with me.

But I maintain the view that it is too difficult to establish a correct understanding of the coordinate schemes from the information available in the repo. Furthermore, I am suspicious that the coordinate situation is overly complex and that there is opportunity for simplification.

This was referenced Oct 28, 2024
Open
Open
Open
Open
Open
Open
Draft
@pjanevskiTT pjanevskiTT force-pushed the pjanevski/soc_desc_harvesting branch 3 times, most recently from a14a415 to 86e2378 Compare October 29, 2024 10:38
@pjanevskiTT
Improve and refactor harvesting and coordinate management
847a37d 
- Coordinate manager class for each arch

- Write tests for the harvesting

- Add functions to convert between coordinate systems
@pjanevskiTT pjanevskiTT merged commit bfa337a into main Oct 29, 2024
18 checks passed
@pjanevskiTT pjanevskiTT deleted the pjanevski/soc_desc_harvesting branch October 29, 2024 14:51
mbezuljTT pushed a commit that referenced this pull request Nov 1, 2024
@pjanevskiTT @mbezuljTT
Soc descriptor harvesting (#202)
d567e50 
- Define base for new harvesting API (coordinate types, translation functions)

- Add tests for the harvesting API

- Implement Blackhole harvesting requirements
This was referenced Nov 5, 2024
Draft
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelsmithTT joelsmithTT joelsmithTT approved these changes

@abhullar-tt abhullar-tt abhullar-tt left review comments

@patrickroberts patrickroberts patrickroberts left review comments

@broskoTT broskoTT broskoTT approved these changes

@vtangTT vtangTT Awaiting requested review from vtangTT

Assignees

@pjanevskiTT pjanevskiTT

Labels
changes api API changing PR, needs changes in client code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Harvesting in SoCDescriptor
5 participants
@pjanevskiTT @abhullar-tt @patrickroberts @joelsmithTT @broskoTT